Skip to content

Add InventoryFilter. Clear orphaned issues.#7165

Merged
labkey-adam merged 5 commits intodevelopfrom
fb_clone7
Oct 31, 2025
Merged

Add InventoryFilter. Clear orphaned issues.#7165
labkey-adam merged 5 commits intodevelopfrom
fb_clone7

Conversation

@labkey-adam
Copy link
Contributor

@labkey-adam labkey-adam commented Oct 30, 2025

Rationale

Refactor domain filtering to allow code sharing between tables with automatic Flag columns and extensible tables (like inventory Location, Box, and Item).

Clean up orphaned issues rows.

@labkey-adam labkey-adam requested review from a team and labkey-matthewb October 30, 2025 22:26
@labkey-adam labkey-adam changed the title Add InventoryFilter Add InventoryFilter. Clear orphaned issues. Oct 31, 2025
// Add a filter to select all rows where the object property with <propertyId> equals the filter value
protected void addObjectPropertyFilter(OrClause orClause, DataFilter filter, TableInfo sourceTable, FieldKey fKey, int propertyId)
{
SQLFragment flagWhere = new SQLFragment("lsid IN (SELECT ObjectURI FROM exp.Object WHERE ObjectId IN (SELECT ObjectId FROM exp.ObjectProperty WHERE StringValue = ? AND PropertyId = ?))", filter.condition().getParamVals()[0], propertyId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since most exp tables have an object it, it might be worth having a parameter that indicates whether to use "lsid" or "objectid"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs the risk of the database actually evaluating ((SELECT ObjectURI FROM exp.Object WHERE ObjectId IN (SELECT ObjectId FROM exp.ObjectProperty WHERE StringValue = ? AND PropertyId = ?)) in a somewhat expensive way. It's index are optimized for objectid=? and propertyid=? joins.
Iff there is a performance issue this might be worth trying

lsid:: WHERE ? = (SELECT StringValue FROM (O WHERE objecturi=lsid) JOIN OP ON objectid WHERE PropertyId=?)
or
objectid:: WHERE ? = (SELECT StringValue FROM OP WHERE OP.ObjectId=.objectid PropertyId=?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since most exp tables have an object it, it might be worth having a parameter that indicates whether to use "lsid" or "objectid"

This is called to filter provisioned tables, none of which have ObjectId (that I've seen). Sample Type tables do have RowId (a material RowId) and that handler generates a custom join that takes advantage. I don't think we can generalize this method to handle all the ways we join into exp.ObjectProperty... in my current FB, I've improved the comments to make it clear this is for cases where LSID is the only option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs the risk of the database actually evaluating ((SELECT ObjectURI FROM exp.Object WHERE ObjectId IN (SELECT ObjectId FROM exp.ObjectProperty WHERE StringValue = ? AND PropertyId = ?)) in a somewhat expensive way. It's index are optimized for objectid=? and propertyid=? joins. Iff there is a performance issue this might be worth trying

lsid:: WHERE ? = (SELECT StringValue FROM (O WHERE objecturi=lsid) JOIN OP ON objectid WHERE PropertyId=?) or objectid:: WHERE ? = (SELECT StringValue FROM OP WHERE OP.ObjectId=.objectid PropertyId=?)

@labkey-matthewb Here's how I've rewritten the full data class provisioned table SELECT statement (with hard-coded values instead of placeholders). Any further suggestions? (Note that because we're constructing and passing around FilterClauses, the first join has to be a sub-select.):

SELECT * FROM expdataclass.c1563d4300_protsequence dc WHERE lsid IN (SELECT ObjectURI FROM exp.Object o INNER JOIN exp.ObjectProperty op ON o.ObjectId = op.ObjectId WHERE StringValue = 'J.POD2' AND PropertyId = 70)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That makes sense. Simpler is usually better (or not worse) so the rewrite “looks” better to me (does that count as vibe coding)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe you're saying this is better:

SELECT * FROM expdataclass.c1563d4300_protsequence dc WHERE 'J.POD2' = (
	SELECT StringValue FROM exp.ObjectProperty op INNER JOIN exp.Object o ON op.ObjectId = o.ObjectId AND ObjectURI = dc.LSID WHERE PropertyId = 70
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s how I would have written it by hand as a first guess (that kinda mirrors what we do for PropertyColumn), but if you have to stuff it into a filter clause, I can see why your rewrite makes sense. I would just keep these ideas in your back pocket if the performance is a problem.


// Helper method that parses a data filter then adds it and its container to the provided collections, coalescing
// cases where multiple containers specify the same filter
static void addDataFilter(String filterName, List<DataFilter> dataFilters, Set<GUID> filteredContainers, GUID guid, String filter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad filter isn't a FilterClause (but I don't know how we get to this point)

Copy link
Contributor

@labkey-matthewb labkey-matthewb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about what metadata/conventions we could add to our schemas to simplify this work.

@labkey-adam labkey-adam merged commit 0516e51 into develop Oct 31, 2025
14 of 15 checks passed
@labkey-adam labkey-adam deleted the fb_clone7 branch October 31, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants